Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slot migration improvement #445

Merged
merged 2 commits into from
May 7, 2024
Merged

Conversation

PingXie
Copy link
Member

@PingXie PingXie commented May 7, 2024

Overview

This PR significantly enhances the reliability and automation of
the Valkey cluster re-sharding process, specifically during slot
migrations in the face of primary failures. These updates address
critical failure issues that previously required extensive manual
intervention and could lead to data loss or inconsistent cluster states.

Enhancements

Automatic Failover Support in Empty Shards

The cluster now supports automatic failover in shards that do not
own any slots, which is common during scaling operations. This
improvement ensures high availability and resilience from the
outset of shard expansion.

Replication of Slot Migration States

All CLUSTER SETSLOT commands are now initially executed on replica
nodes before the primary. This ensures that the slot migration state
is consistent within the shard, preventing state loss in the event of
primary failure. A new timeout parameter has been introduced, allowing
users to specify the duration in milliseconds to wait for replication
to complete, with a default set at 2 seconds.

CLUSTER SETSLOT slot { IMPORTING node-id | MIGRATING node-id | NODE node-id | STABLE } [ TIMEOUT timeout ]

Recovery of Logical Migration Links

The update automatically repairs the logical links between source
and target nodes during failovers. This ensures that requests are
correctly redirected to the new primary in the target shard after
a primary failure, maintaining cluster integrity.

Enhanced Support for New Replicas

New replicas added to shards involved in slot migrations will now
automatically inherit the slot's migration state as part of their
initialization. This ensures that new replicas are immediately
consistent with the rest of the shard.

Improved Logging for Slot Migrations

Additional logging has been implemented to provide operators with
clearer insights into the slot migration processes and automatic
recovery actions, aiding in monitoring and troubleshooting.

Additional Changes

cluster-allow-replica-migration

When cluster-allow-replica-migration is disabled, primary nodes that
lose their last slot to another shard will no longer automatically become
replicas of the receiving shard. Instead, they will remain in their own
shards, which will now be empty, having no slots assigned to them.

Fixes #21.

…ding process,

specifically during slot migrations in the face of primary failures.

Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@google.com>
Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 82.82443% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 68.89%. Comparing base (93f8a19) to head (fa0285c).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #445      +/-   ##
============================================
+ Coverage     68.43%   68.89%   +0.45%     
============================================
  Files           109      109              
  Lines         61681    61785     +104     
============================================
+ Hits          42214    42565     +351     
+ Misses        19467    19220     -247     
Files Coverage Δ
src/commands.def 100.00% <ø> (ø)
src/debug.c 53.47% <100.00%> (+0.65%) ⬆️
src/networking.c 85.08% <100.00%> (ø)
src/rdb.c 76.22% <100.00%> (-0.05%) ⬇️
src/replication.c 86.01% <100.00%> (-0.29%) ⬇️
src/server.c 88.60% <100.00%> (+0.47%) ⬆️
src/blocked.c 91.80% <91.66%> (-0.06%) ⬇️
src/cluster_legacy.c 83.16% <81.11%> (+8.46%) ⬆️

... and 12 files with indirect coverage changes

@PingXie PingXie merged commit 6e7af94 into valkey-io:unstable May 7, 2024
18 checks passed
@enjoy-binbin
Copy link
Member

i see the (squash merge) commit message is missing (Signed-off-by footer is also missing)

@srgsanky
Copy link
Contributor

srgsanky commented May 8, 2024

Does any of the CI runs do make test-cluster? We might want to enable cluster tests in at least one run @madolson

I am able to consistently get a test failure after this commit. @PingXie can you please take a look?

./runtest-cluster --single tests/12-replica-migration-2

@madolson
Copy link
Member

madolson commented May 8, 2024

@srgsanky We're running most of the cluster tests as part of #442.

zuiderkwast added a commit that referenced this pull request May 24, 2024
After READONLY, make a cluster replica behave as its primary regarding
returning ASK redirects and TRYAGAIN.

Without this patch, a client reading from a replica cannot tell if a key
doesn't exist or if it has already been migrated to another shard as
part of an ongoing slot migration. Therefore, without an ASK redirect in
this situation, offloading reads to cluster replicas wasn't reliable.

Note: The target of a redirect is always a primary. If a client wants to
continue reading from a replica after following a redirect, it needs to
figure out the replicas of that new primary using CLUSTER SHARDS or
similar.

This is related to #21 and has been made possible by the introduction of
Replication of Slot Migration States in #445.

----

Release notes:

During cluster slot migration, replicas are able to return -ASK
redirects and -TRYAGAIN.

---------

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
PingXie added a commit that referenced this pull request Jul 16, 2024
Fixes a regression introduced in PR #445, which allowed a message from a
replica
to update the slot ownership of its primary. The regression results in a
`replicaof` cycle, causing server crashes due to the cycle detection
assert. The
fix restores the previous behavior where only primary senders can
trigger
`clusterUpdateSlotsConfigWith`.

Additional changes:

* Handling of primaries without slots is obsoleted by new handling of
when a
  sender that was a replica announces that it is now a primary.
* Replication loop detection code is unchanged but shifted downwards.
* Some variables are renamed for better readability and some are
introduced to
  avoid repeated memcmp() calls.

Fixes #753.

---------

Signed-off-by: Ping Xie <pingxie@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Improve slot migration reliability
4 participants